Add SDK support for perfmap format version #19132
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR addresses the remaining work outlined in the issue https://github.com/dotnet/sdk/issues/18813 As of .NET 6 Preview 7 Crossgen2 supports the option --perfmap-format-version that currently supports two values, 0 (being the legacy value / default) and 1; the only difference is that version 1 uses a different naming scheme for the output perfmap files, dropping the {MVID} part and using the extension ".ni.r2rmap". This PR adds support for the new option in the SDK Crossgen2 publishing logic; for .NET 5 and prior, we naturally need to stick to the legacy naming; for .NET 6, SDK honors a new property PublishReadyToRunPerfmapFormatVersion that can be used to override the default. As of the 1st commit on this PR, the SDK default is set to 1 i.e. the "new format". This basically means that from the point of merging this PR onwards SDK will by default produce the new perfmap file names on Linux. Similarly, once SDK preview 7 combines with the runtime repo (which will likely happen as part of the RC1 fork), the installer partition will start producing the new naming style for the Crossgen2-compiled framework. As this is technically a breaking change, we need to discuss whether that's acceptable (e.g. if there was no prior support for perfmap indexation, we probably don't need to care much); if we need tighter control over the perfmap versioning / naming, there are two different things we can do: 1) For now change the PerfmapFormatVersion default to 0 in the SDK Microsoft.NET.CrossGen.targets script. This means that perfmap files will continue to use the legacy naming scheme until someone explicitly switches over runtime or the SDK itself later on in the future. 2) We can make a counterpart check-in to the runtime repo setting PublishReadyToRunPerfmapFormatVersion to 0 in the installer ReadyToRun.targets script; once we're confident to switch over, we'll just delete the property from the scripts. I'm still hitting a couple of errors in local testing but I'm publishing the PR anyway as I'm hoping to solicit early feedback for the change. Thanks Tomas
trylek
requested review from
brianrob,
mikem8361,
davidwrighton,
hoyosjs and
AntonLapounov
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
brianrob
approved these changes
Jul 22, 2021
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the remaining work outlined in the issue
https://github.com/dotnet/sdk/issues/18813
As of .NET 6 Preview 7 Crossgen2 supports the option
--perfmap-format-version that currently supports two values, 0
(being the legacy value / default) and 1; the only difference is
that version 1 uses a different naming scheme for the output
perfmap files, dropping the {MVID} part and using the extension
".ni.r2rmap".
This PR adds support for the new option in the SDK Crossgen2
publishing logic; for .NET 5 and prior, we naturally need to stick
to the legacy naming; for .NET 6, SDK honors a new property
PublishReadyToRunPerfmapFormatVersion that can be used to override
the default.
As of the 1st commit on this PR, the SDK default is set to 1 i.e.
the "new format". This basically means that from the point of
merging this PR onwards SDK will by default produce the new
perfmap file names on Linux.
Similarly, once the current SDK combines with the runtime repo
(which will likely happen as part of the RC1 fork), the installer
partition will start producing the new naming style for the
Crossgen2-compiled framework.
As this is technically a breaking change, we need to discuss whether
that's acceptable (e.g. if there was no prior support for perfmap
indexation, we probably don't need to care much); if we need tighter
control over the perfmap versioning / naming, there are two different
things we can do:
For now change the PublishReadyToRunPerfmapFormatVersion default
to 0 in the SDK Microsoft.NET.CrossGen.targets script. This means that
perfmap files will continue to use the legacy naming scheme until someone
explicitly switches over runtime or the SDK itself later on in the future.
We can make a counterpart check-in to the runtime repo setting
PublishReadyToRunPerfmapFormatVersion to 0 in the installer
ReadyToRun.targets script; once we're confident to switch over,
we'll just delete the property from the script.
I'm still hitting a couple of errors in local testing but I'm
publishing the PR anyway as I'm hoping to solicit early feedback for the
change.
Thanks
Tomas
/cc @dotnet/crossgen-contrib